Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enabling the user to chose between complain and enforce mode in 1.6.1.3. #94

Conversation

bgro
Copy link
Contributor

@bgro bgro commented Sep 22, 2023

Overall Review of Changes:
Control 1.6.1.3 mandates to Ensure all AppArmor Profiles are in enforce or complain mode.
However, the corresponding task only allows the role to set every profile to enforce mode --
the existing toggle in defaults/main.yml disables the tasks rather than switching between
enforce and complain mode.

This fix changes the implementation such that complain or enforce mode can be chosen.

Issue Fixes:

@bgro
Copy link
Contributor Author

bgro commented Sep 22, 2023

Hold on, I overlooked the dependency with 1.6.1.4.

… of rules 1.6.1.4 and 1.6.1.3 in the task file. This is necessary because the two rules set the same value, but if both rules are selected, e.g., when using tags for Level 1 and Level 2 (as is necessary when aiming for L2 compliance), then rule 1.6.1.4 must take precedence.

Signed-off-by: Bernd Grobauer <[email protected]>
…his could be removed once the audit role is updated.

Signed-off-by: Bernd Grobauer <[email protected]>
@uk-bolly
Copy link
Member

Hi @bgro

Superb PR and work thank you so much.
I am currently testing locally, however i have noticed it will fail yamllint checks. The default is set to 4 spaces (being consistent also helps the ansible-galaxy score). I have run it to feedback. If you could amend accordingly that would be excellent.

tasks/prelim.yml
  4:5       error    wrong indentation: expected 6 but found 4  (indentation)
  8:1       error    too many blank lines (2 > 1)  (empty-lines)

tasks/section_1/cis_1.6.x.yml
  69:11     error    wrong indentation: expected 12 but found 10  (indentation)
  111:11    error    wrong indentation: expected 12 but found 10  (indentation)
  114:11    error    wrong indentation: expected 12 but found 10  (indentation)
  117:11    error    wrong indentation: expected 12 but found 10  (indentation)
  120:11    error    wrong indentation: expected 12 but found 10  (indentation)
  154:1     error    too many blank lines (1 > 0)  (empty-lines)

many thanks

uk-bolly

@uk-bolly
Copy link
Member

hi @bgro

To get around enforce always benig set even on level 2 could i suggest that

- ubtu22cis_apparmor_mode == "enforce"

is added to line 98, that would get around it forcing something not requested in error.

Again great work.

Thanks

uk-bolly

@bgro
Copy link
Contributor Author

bgro commented Sep 22, 2023

Hi @uk-bolly,

I forgot about linting, sorry!

Regarding your suggestion to add

- ubtu22cis_apparmor_mode == "enforce"

I think I understand that you want this, because the control can really be invasive and break stuff, and therefore you want an extra variable to disable or at least defang rule 1.6.1.4, yes?

That would lead to either nothing being done, if 1.6.1.3 is not part of what needs to be executed (e.g., wenn only executing L2 rules) or 1.6.1.3 being executed instead...

I think that is not really ideal. Probably the easiest would be make 1.6.1.4 to also use ubtu22cis_apparmor_mode for
implementing, but not for checking (and document accordingly, i.e., have

ansible.builtin.shell: aa-{{ubtu22_cis_apparmor_mode}} /etc/apparmor.d/*

in line 80.

What do you think about that?

Update: Ah, there are, of course no AUDITs performed by this rule, the other stuff is about idempotency.
OK, how about making 1.6.1.3 almost the same as 1.6.1.4, but in case complain is chosen, add a warning
regarding non-compliance with 1.6.1.4?

Signed-off-by: Bernd Grobauer <[email protected]>
@uk-bolly
Copy link
Member

Hi @uk-bolly,

I forgot about linting, sorry!

Regarding your suggestion to add

- ubtu22cis_apparmor_mode == "enforce"

I think I understand that you want this, because the control can really be invasive and break stuff, and therefore you want an extra variable to disable or at least defang rule 1.6.1.4, yes?

That would lead to either nothing being done, if 1.6.1.3 is not part of what needs to be executed (e.g., wenn only executing L2 rules) or 1.6.1.3 being executed instead...

I think that is not really ideal. Probably the easiest would be make 1.6.1.4 to also use ubtu22cis_apparmor_mode for implementing, but not for checking (and document accordingly, i.e., have

ansible.builtin.shell: aa-{{ubtu22_cis_apparmor_mode}} /etc/apparmor.d/*

in line 80.

What do you think about that?

Update: Ah, there are, of course no AUDITs performed by this rule, the other stuff is about idempotency. OK, how about making 1.6.1.3 almost the same as 1.6.1.4, but in case complain is chosen, add a warning regarding non-compliance with 1.6.1.4?

hi @bgro

Been looking and thinking about this one for a while. Ideally we should only do what the control asks for. Like the rest of the tasks they should only run if required. i am happy to skip 1.6.1.3 if 1.6.1.4 is set and running. We should set a warning though if only level 1 is run.
This could get a little more complicated still. Let me discuss this with the team and see what othewr ideas we can come up with.
Many thanks

uk-bolly

@uk-bolly
Copy link
Member

hi @bgro

Thank you for this PR as always. I can see it is still failing on pre-commit checks, if you are able to resolve this would like to get this merged.

Many thanks

uk-bolly

@bgro
Copy link
Contributor Author

bgro commented Oct 20, 2023

HI @uk-bolly, I made a new PR #148 (I was not quite sure about updating the old one with rebase and everything). The syntax issues are gone, but the pipeline now terminates with an error for what seems like some infrastructure issue. What do I need to do to re-run the pipeline?

@uk-bolly
Copy link
Member

hi @bgro

i think we are all sorted now? I believe this PR can be closed now?

many thanks once again for all of your help.

uk-bolly

@uk-bolly
Copy link
Member

uk-bolly commented Nov 9, 2023

closing as replaced by #148

@uk-bolly uk-bolly closed this Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants